-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(navBar): update inkbar on screen resize #11042
Conversation
Splaktar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had some minor change requests and a note that this may end up being a breaking change for some apps. We'll see how the presubmit tests go once the code review comments are addressed. Hopefully no one is using those private classes...
|
Oh and I apologize for taking so long to get to this review! |
|
@chmelevskij I would like to get this presubmitted and merged in a release pretty soon. Do you think that you will be able to address the review feedback soon? |
|
Hi there, will do that tomorrow, and hopefully submit the changes by the end of this week/ beginning of the next. |
|
Excellent, thank you! |
|
Pushed the changes from review. Should I squash the commits and make a new PR? |
|
@chmelevskij thank you for making those updates. Please squash the commits and then force push them up to this same branch. That will update this PR. We shouldn't need to create a new PR. Sorry for the delay, I just returned from a couple conferences. |
Splaktar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you got all of the requested changes but one. Also can you please remove the changes to the package-lock.json?
|
It's possible that this could go into 1.1.10 which we plan to release soon. Can you please address the comments above? |
|
@chmelevskij can you please do a quick rebase and force push to your branch? Thank you. |
240ba41 to
d73d2bb
Compare
Changed how inkbar is moved. Using transform to scale and translate the inkbar. Also added debounced update to window resize. Closes angular#10121
d73d2bb to
a08b9cd
Compare
| '</div>', | ||
| link: function(scope, element, attrs, ctrl) { | ||
|
|
||
| ctrl.width = $window.innerWidth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the test and lint failures, it appears that $window needs to be injected above into
function MdNavBar($mdAria, $mdTheming) {
| angular.element($window).off('resize', onResize); | ||
| } | ||
|
|
||
| angular.element($window).on('resize', $mdUtil.debounce(onResize, 300)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like $mdUtil needs to be injected too.
|
Thank you for rebasing, but there are some lint and build errors that need to be resolved now. |
|
Fixed rebase/lint issues in PR #11912. Closing this one to continue the work there. |
Changed how inkbar is moved. Using transform to scale and translate the
inkbar. Also added debounced update to window resize.
Closes #10121
PR Checklist
Please check that your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #10121 mdInkBar doesn't update on screen resize.
What is the new behavior?
Debounced update of the mdInkBar.
Does this PR introduce a breaking change?
Other information